[MCA][X86] Pretend To Have a Stack Engine #153348
Conversation
|
@llvm/pr-subscribers-backend-x86 Author: Aiden Grossman (boomanaiden154) ChangesThis patch removes RSP dependencies from push and pop instructions to pretend that we have a stack engine. This does not model details like sync uops that are relevant implementation details due to complexity. This is just enabled on all X86 CPUs given LLVM does not have a scheduling model for any X86 CPU that does not have a stack engine. This fixes #152008. Full diff: https://github.com/llvm/llvm-project/pull/153348.diff 4 Files Affected:
diff --git a/llvm/lib/Target/X86/MCA/X86CustomBehaviour.cpp b/llvm/lib/Target/X86/MCA/X86CustomBehaviour.cpp
index 817e88d8a0bc0..af3931f896107 100644
--- a/llvm/lib/Target/X86/MCA/X86CustomBehaviour.cpp
+++ b/llvm/lib/Target/X86/MCA/X86CustomBehaviour.cpp
@@ -36,11 +36,31 @@ void X86InstrPostProcess::setMemBarriers(std::unique_ptr<Instruction> &Inst,
}
}
+void X86InstrPostProcess::useStackEngine(std::unique_ptr<Instruction> &Inst,
+ const MCInst &MCI) {
+ if (X86::isPOP(MCI.getOpcode())) {
+ assert(Inst->getUses().size() == 1 &&
+ "Expected pop instruction to only use stack pointer register");
+ Inst->getUses().clear();
+ }
+ if (X86::isPUSH(MCI.getOpcode())) {
+ auto *StackRegisterUse =
+ llvm::find_if(Inst->getUses(), [](const ReadState &State) {
+ return State.getRegisterID() == X86::RSP;
+ });
+ assert(
+ StackRegisterUse != Inst->getUses().end() &&
+ "Expected push instruction to implicitly use stack pointer register.");
+ Inst->getUses().erase(StackRegisterUse);
+ }
+}
+
void X86InstrPostProcess::postProcessInstruction(
std::unique_ptr<Instruction> &Inst, const MCInst &MCI) {
// Currently, we only modify certain instructions' IsALoadBarrier and
// IsAStoreBarrier flags.
setMemBarriers(Inst, MCI);
+ useStackEngine(Inst, MCI);
}
} // namespace mca
diff --git a/llvm/lib/Target/X86/MCA/X86CustomBehaviour.h b/llvm/lib/Target/X86/MCA/X86CustomBehaviour.h
index 4a83ba848dd88..c5459e42dfc9f 100644
--- a/llvm/lib/Target/X86/MCA/X86CustomBehaviour.h
+++ b/llvm/lib/Target/X86/MCA/X86CustomBehaviour.h
@@ -28,6 +28,11 @@ class X86InstrPostProcess : public InstrPostProcess {
/// as load and store barriers.
void setMemBarriers(std::unique_ptr<Instruction> &Inst, const MCInst &MCI);
+ /// Called within X86InstrPostPorcess to remove some rsp read operands
+ /// on stack instructions to better simulate the stack engine. We currently
+ /// do not model features of the stack engine like sync uops.
+ void useStackEngine(std::unique_ptr<Instruction> &Inst, const MCInst &MCI);
+
public:
X86InstrPostProcess(const MCSubtargetInfo &STI, const MCInstrInfo &MCII)
: InstrPostProcess(STI, MCII) {}
diff --git a/llvm/test/tools/llvm-mca/X86/stack-engine-pop.s b/llvm/test/tools/llvm-mca/X86/stack-engine-pop.s
new file mode 100644
index 0000000000000..9eae41b7af060
--- /dev/null
+++ b/llvm/test/tools/llvm-mca/X86/stack-engine-pop.s
@@ -0,0 +1,81 @@
+# NOTE: Assertions have been autogenerated by utils/update_mca_test_checks.py
+# RUN: llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=skylake -timeline -iterations=1 < %s | FileCheck %s
+
+popq %rax
+popq %rcx
+popq %rdx
+popq %rbx
+popq %r12
+
+# CHECK: Iterations: 1
+# CHECK-NEXT: Instructions: 5
+# CHECK-NEXT: Total Cycles: 11
+# CHECK-NEXT: Total uOps: 10
+
+# CHECK: Dispatch Width: 6
+# CHECK-NEXT: uOps Per Cycle: 0.91
+# CHECK-NEXT: IPC: 0.45
+# CHECK-NEXT: Block RThroughput: 2.5
+
+# CHECK: Instruction Info:
+# CHECK-NEXT: [1]: #uOps
+# CHECK-NEXT: [2]: Latency
+# CHECK-NEXT: [3]: RThroughput
+# CHECK-NEXT: [4]: MayLoad
+# CHECK-NEXT: [5]: MayStore
+# CHECK-NEXT: [6]: HasSideEffects (U)
+
+# CHECK: [1] [2] [3] [4] [5] [6] Instructions:
+# CHECK-NEXT: 2 6 0.50 * popq %rax
+# CHECK-NEXT: 2 6 0.50 * popq %rcx
+# CHECK-NEXT: 2 6 0.50 * popq %rdx
+# CHECK-NEXT: 2 6 0.50 * popq %rbx
+# CHECK-NEXT: 2 6 0.50 * popq %r12
+
+# CHECK: Resources:
+# CHECK-NEXT: [0] - SKLDivider
+# CHECK-NEXT: [1] - SKLFPDivider
+# CHECK-NEXT: [2] - SKLPort0
+# CHECK-NEXT: [3] - SKLPort1
+# CHECK-NEXT: [4] - SKLPort2
+# CHECK-NEXT: [5] - SKLPort3
+# CHECK-NEXT: [6] - SKLPort4
+# CHECK-NEXT: [7] - SKLPort5
+# CHECK-NEXT: [8] - SKLPort6
+# CHECK-NEXT: [9] - SKLPort7
+
+# CHECK: Resource pressure per iteration:
+# CHECK-NEXT: [0] [1] [2] [3] [4] [5] [6] [7] [8] [9]
+# CHECK-NEXT: - - 1.00 1.00 2.00 3.00 - 1.00 2.00 -
+
+# CHECK: Resource pressure by instruction:
+# CHECK-NEXT: [0] [1] [2] [3] [4] [5] [6] [7] [8] [9] Instructions:
+# CHECK-NEXT: - - - - - 1.00 - - 1.00 - popq %rax
+# CHECK-NEXT: - - - - 1.00 - - 1.00 - - popq %rcx
+# CHECK-NEXT: - - - 1.00 - 1.00 - - - - popq %rdx
+# CHECK-NEXT: - - 1.00 - 1.00 - - - - - popq %rbx
+# CHECK-NEXT: - - - - - 1.00 - - 1.00 - popq %r12
+
+# CHECK: Timeline view:
+# CHECK-NEXT: 0
+# CHECK-NEXT: Index 0123456789
+
+# CHECK: [0,0] DeeeeeeER . popq %rax
+# CHECK-NEXT: [0,1] DeeeeeeER . popq %rcx
+# CHECK-NEXT: [0,2] D=eeeeeeER. popq %rdx
+# CHECK-NEXT: [0,3] .DeeeeeeER. popq %rbx
+# CHECK-NEXT: [0,4] .D=eeeeeeER popq %r12
+
+# CHECK: Average Wait times (based on the timeline view):
+# CHECK-NEXT: [0]: Executions
+# CHECK-NEXT: [1]: Average time spent waiting in a scheduler's queue
+# CHECK-NEXT: [2]: Average time spent waiting in a scheduler's queue while ready
+# CHECK-NEXT: [3]: Average time elapsed from WB until retire stage
+
+# CHECK: [0] [1] [2] [3]
+# CHECK-NEXT: 0. 1 1.0 1.0 0.0 popq %rax
+# CHECK-NEXT: 1. 1 1.0 1.0 0.0 popq %rcx
+# CHECK-NEXT: 2. 1 2.0 2.0 0.0 popq %rdx
+# CHECK-NEXT: 3. 1 1.0 1.0 0.0 popq %rbx
+# CHECK-NEXT: 4. 1 2.0 2.0 0.0 popq %r12
+# CHECK-NEXT: 1 1.4 1.4 0.0 <total>
diff --git a/llvm/test/tools/llvm-mca/X86/stack-engine-push.s b/llvm/test/tools/llvm-mca/X86/stack-engine-push.s
new file mode 100644
index 0000000000000..8ba4e875203b4
--- /dev/null
+++ b/llvm/test/tools/llvm-mca/X86/stack-engine-push.s
@@ -0,0 +1,80 @@
+# NOTE: Assertions have been autogenerated by utils/update_mca_test_checks.py
+# RUN: llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=skylake -timeline -iterations=1 < %s | FileCheck %s
+
+pushq %rax
+pushq %rcx
+pushq %rdx
+pushq %rbx
+pushq %r12
+
+# CHECK: Iterations: 1
+# CHECK-NEXT: Instructions: 5
+# CHECK-NEXT: Total Cycles: 9
+# CHECK-NEXT: Total uOps: 15
+
+# CHECK: Dispatch Width: 6
+# CHECK-NEXT: uOps Per Cycle: 1.67
+# CHECK-NEXT: IPC: 0.56
+# CHECK-NEXT: Block RThroughput: 5.0
+
+# CHECK: Instruction Info:
+# CHECK-NEXT: [1]: #uOps
+# CHECK-NEXT: [2]: Latency
+# CHECK-NEXT: [3]: RThroughput
+# CHECK-NEXT: [4]: MayLoad
+# CHECK-NEXT: [5]: MayStore
+# CHECK-NEXT: [6]: HasSideEffects (U)
+
+# CHECK: [1] [2] [3] [4] [5] [6] Instructions:
+# CHECK-NEXT: 3 2 1.00 * pushq %rax
+# CHECK-NEXT: 3 2 1.00 * pushq %rcx
+# CHECK-NEXT: 3 2 1.00 * pushq %rdx
+# CHECK-NEXT: 3 2 1.00 * pushq %rbx
+# CHECK-NEXT: 3 2 1.00 * pushq %r12
+
+# CHECK: Resources:
+# CHECK-NEXT: [0] - SKLDivider
+# CHECK-NEXT: [1] - SKLFPDivider
+# CHECK-NEXT: [2] - SKLPort0
+# CHECK-NEXT: [3] - SKLPort1
+# CHECK-NEXT: [4] - SKLPort2
+# CHECK-NEXT: [5] - SKLPort3
+# CHECK-NEXT: [6] - SKLPort4
+# CHECK-NEXT: [7] - SKLPort5
+# CHECK-NEXT: [8] - SKLPort6
+# CHECK-NEXT: [9] - SKLPort7
+
+# CHECK: Resource pressure per iteration:
+# CHECK-NEXT: [0] [1] [2] [3] [4] [5] [6] [7] [8] [9]
+# CHECK-NEXT: - - 1.00 1.00 1.00 2.00 5.00 1.00 2.00 2.00
+
+# CHECK: Resource pressure by instruction:
+# CHECK-NEXT: [0] [1] [2] [3] [4] [5] [6] [7] [8] [9] Instructions:
+# CHECK-NEXT: - - - - - - 1.00 - 1.00 1.00 pushq %rax
+# CHECK-NEXT: - - - - - 1.00 1.00 1.00 - - pushq %rcx
+# CHECK-NEXT: - - - 1.00 1.00 - 1.00 - - - pushq %rdx
+# CHECK-NEXT: - - 1.00 - - - 1.00 - - 1.00 pushq %rbx
+# CHECK-NEXT: - - - - - 1.00 1.00 - 1.00 - pushq %r12
+
+# CHECK: Timeline view:
+# CHECK-NEXT: Index 012345678
+
+# CHECK: [0,0] DeeER. . pushq %rax
+# CHECK-NEXT: [0,1] D=eeER . pushq %rcx
+# CHECK-NEXT: [0,2] .D=eeER . pushq %rdx
+# CHECK-NEXT: [0,3] .D==eeER. pushq %rbx
+# CHECK-NEXT: [0,4] . D==eeER pushq %r12
+
+# CHECK: Average Wait times (based on the timeline view):
+# CHECK-NEXT: [0]: Executions
+# CHECK-NEXT: [1]: Average time spent waiting in a scheduler's queue
+# CHECK-NEXT: [2]: Average time spent waiting in a scheduler's queue while ready
+# CHECK-NEXT: [3]: Average time elapsed from WB until retire stage
+
+# CHECK: [0] [1] [2] [3]
+# CHECK-NEXT: 0. 1 1.0 1.0 0.0 pushq %rax
+# CHECK-NEXT: 1. 1 2.0 1.0 0.0 pushq %rcx
+# CHECK-NEXT: 2. 1 2.0 1.0 0.0 pushq %rdx
+# CHECK-NEXT: 3. 1 3.0 1.0 0.0 pushq %rbx
+# CHECK-NEXT: 4. 1 3.0 1.0 0.0 pushq %r12
+# CHECK-NEXT: 1 2.2 1.0 0.0 <total>
|
| if (X86::isPOP(MCI.getOpcode())) { | ||
| assert(Inst->getUses().size() == 1 && | ||
| "Expected pop instruction to only use stack pointer register"); | ||
| Inst->getUses().clear(); | ||
| } |
There was a problem hiding this comment.
A use of RSP is "free" only if the definition of RSP comes from another PUSH/POP/RET. That is because the stack engine only tracks RSP updates from those known stack opcodes. The stack engine computes a delta between the new RSP value and the old RSP value. That delta is then used as an implicit addend operand to other RSP reads performed by other stack operations.
The first time a stack operation is encountered, RSP should be treated like a normal read dependency, and we would pay the full cost of waiting on RSP. Any other following stack operations will be able to eliminate their dependency from RSP.
A write to RSP which isn't from a stack operation would force a synchronization. So, the next stack operation won't be able to benefit from that optimization. We would basically need to go back to square one.
In your case:
- if the last definition of RSP is unknown (we just started simulating), or it is not coming from another PUSH/POP/RET, then we should pay the full dependency cost.
- if there is a definition of RSP which isn't coming from a stack operation, then the next PUSH/POP/RET will pay the full cost of waiting on RSP.
Bottom line: your change is mostly OK. However, you need to add some sort of post-processing state/context;
as you post-process instructions, you should record where the last definition of RSP came from. Your logic would only eliminate the dependency if the definition was coming from a known stack operation.
I hope it makes sense.
-Andrea
There was a problem hiding this comment.
Ah, true.
This ended up being more complicated than I thought. We can't just disable the read dependency for instructions following an initial stack operation because then the subsequent stack operations can execute out of order with an instruction setting rsp. Getting rid of the write would theoretically work, but would require modifying the previous instruction inside the instruction processor (if we see the current instruction is also a stack operation), which doesn't fit well within an MCAD type workflow like the one we're using.
I'll have to think about this one a bit more and figure out what would actually work.
There was a problem hiding this comment.
PUSH instructions are essential store operations, and they are guaranteed to always be processed in-order by the LSUnit (see method dispatch() https://github.com/llvm/llvm-project/blob/main/llvm/lib/MCA/HardwareUnits/LSUnit.cpp#L69).
However, POP instructions are problematic because they behave like normal load operations, and the LSUnit allows loads to be reordered w.r.t. other loads (see code below).
// A load may pass a previous load.
MemoryGroup &Group = getGroup(CurrentLoadGroupID);
Group.addInstruction();
return CurrentLoadGroupID;
Before your patch, this was fine because POP instructions were also in a register dependency due to the RSP updates. So, although technically they didn't alias with each other (i.e. they were part of a same load group), they were never scheduled out of order.
You could probably workaround it by forcing stack load (e.g. POP) to be in their own load group. Groups are guaranteed to execute in-order, so that would force POP instruction to always execute in the right sequence. Note however that this isn't an ideal/perfect solution, because it prevents unrelated non-stack loads to be reordered w.r.t. POP instructions.
There was a problem hiding this comment.
I don't remember the custom-behaviour / instr postprocess mechanism.
However, I think that we can add artificial dependencies (similar to chain deps in DAG) to force POP instructions to always run in sequence. Maybe @mshockwave knows the answer to it.
There was a problem hiding this comment.
I think I made a bit of a mountain out of a mole hill.
The stack engine computes the rsp offset right after decode time. So if we have another operation after some push/pop instructions that uses rsp, the decoder generates a stack sync uop to sync the state held in the decoder which the instruction using/writing rsp then depends on. The dependency chain for the rsp using/writing instruction does not actually end up depending on the stack operations writing anything back.
This means that in MCA we can just clear writes to rsp in stack operation instructions.
UICA models it this way:
I think that is reasonably close to real hardware, although with the snippet uica is simulating above, I'm getting a reciprocal throughput of three cycles instead of 2.5 like uica simulates:
# LLVM-EXEGESIS-MEM-MAP test1 131072
movq $0x20030, %rsp
popq %rax
popq %rcx
popq %rdx
popq %rbx
popq %r12
(~10900 cycles with min-instructions=20000, ~5900 cycles with min-instructions=10000, 1667 iterations per 10000 instructions gives a reciprocal throughput of about 3).
So uica isn't completely accurate here, but the reciprocal throughput of this being three cycles I'm reasonably sure implies there can't be a blocking write dependency here.
As a side note it looks like the scheduling model for pushq on skylake is not accurate, but that's a separate problem.
There was a problem hiding this comment.
generates a stack sync uop to sync the state held in the decoder which the instruction using/writing rsp then depends on
So the decoder would basically "embedded" the (new) offset into this sync uop, that's why you said it no longer depends on any push / pop?
There was a problem hiding this comment.
Yeah. At least that's my understanding of it.
There was a problem hiding this comment.
I think I made a bit of a mountain out of a mole hill.
This means that in MCA we can just clear writes to rsp in stack operation instructions.
I think it is reasonable. However, I think it would be even better if we don't always blindly eliminate those write.
My point is that you should keep a write if the next RSP access is a read which isn't coming from a stack operation. That way, we better simulate situations where a sync is required.
There was a problem hiding this comment.
(My previous example on uiCA was bad, because the register would just get renamed. Here it models stack sync uops).
Keeping the writes around for the stack operations would mean that the add instruction in the second iteration could only finish executing after the last pop instruction in the first iteration. While completely ignoring the stack sync uop does makes us less accurate here, keeping the write would make us significantly less accurate.
| "Expected pop instruction to only use stack pointer register"); | ||
| Inst->getUses().clear(); | ||
| } | ||
| if (X86::isPUSH(MCI.getOpcode())) { |
There was a problem hiding this comment.
NOTE: there is also PUSHF and POPF (for the purpose of this analysis, you can treat them like normal PUSH/POP).
Ignore RET for now.. that's problematic for other reasons, and we don't simulate it properly.
Edit: on a second thought, we don't know whether PUSHF/POPF are also optimized, or whether they force a synchronization. That needs to be tested on real hw. For now, I think you can ignore them.
There was a problem hiding this comment.
Added a TODO to do actual benchmarking. Ignoring them for now sounds good.
This patch removes RSP dependencies from push and pop instructions to pretend that we have a stack engine. This does not model details like sync uops that are relevant implementation details due to complexity. This is just enabled on all X86 CPUs given LLVM does not have a scheduling model for any X86 CPU that does not have a stack engine. This fixes llvm#152008.
boomanaiden154
force-pushed
the
llvm-mca-x86-stack-engine
branch
from
b78c89e to
aa7076c
Compare
| if (X86::isPOP(MCI.getOpcode())) { | ||
| assert(Inst->getUses().size() == 1 && | ||
| "Expected pop instruction to only use stack pointer register"); | ||
| Inst->getUses().clear(); | ||
| } |
There was a problem hiding this comment.
generates a stack sync uop to sync the state held in the decoder which the instruction using/writing rsp then depends on
So the decoder would basically "embedded" the (new) offset into this sync uop, that's why you said it no longer depends on any push / pop?
This patch removes RSP dependencies from push and pop instructions to pretend that we have a stack engine. This does not model details like sync uops that are relevant implementation details due to complexity. This is just enabled on all X86 CPUs given LLVM does not have a scheduling model for any X86 CPU that does not have a stack engine.
This fixes #152008.